Skip to content

Fix requires_lto targets needing lto set in cargo #149624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix requires_lto targets needing lto set in cargo #149624

wants to merge 2 commits into from
+52 −7

Conversation

@Flakebi
Copy link
Contributor

@Flakebi Flakebi commented Dec 4, 2025 •
edited by rustbot
Loading

Targets that set requires_lto = true were not actually using lto when compiling with cargo by default. They needed an extra lto = true in Cargo.toml to work.

Fix this by letting lto take precedence over the embed_bitcode flag when lto is required by a target.

If both these flags would be supplied by the user, an error is generated. However, this did not happen when lto was requested by the target instead of the user.

Fixes #148514
Tracking issue: #135024

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@Flakebi Flakebi mentioned this pull request Dec 4, 2025
Open
@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2025

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

@Flakebi Flakebi mentioned this pull request Dec 4, 2025
Open
22 tasks
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu
Copy link
Member

jieyouxu commented Dec 4, 2025

Hm, not entirely sure of the implications.
@rustbot reroll

@rustbot rustbot assigned Mark-Simulacrum and unassigned jieyouxu Dec 4, 2025
@Flakebi
Copy link
Contributor Author

Flakebi commented Dec 4, 2025

The CI/tidy complain seems like a false-positive, the test uses --target, just not through //@ compile-flags:. Maybe that lint shouldn’t run on run-make-cargo tests?
(I’m happy to make the change, just want to get confirmation that it’s ok to do that.)

@jieyouxu
Copy link
Member

jieyouxu commented Dec 4, 2025 •
edited
Loading

The CI/tidy complain seems like a false-positive, the test uses --target, just not through //@ compile-flags:. Maybe that lint shouldn’t run on run-make-cargo tests? (I’m happy to make the change, just want to get confirmation that it’s ok to do that.)

Yes, I probably forgot to extend the tidy exception to run-make-cargo when I split the previous run-make test suite.

@Flakebi
Ignore tidy target check in run-make-cargo
66812a3 
run-make-cargo tests can specify the target in ways we do not detect in
the check, so disable the check there.
@rustbot rustbot added A-tidy Area: The tidy tool T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 4, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Flakebi
Fix requires_lto targets needing lto set in cargo
2dd6d17 
Targets that set `requires_lto = true` were not actually using lto when
compiling with cargo by default. They needed an extra `lto = true` in
`Cargo.toml` to work.

Fix this by letting lto take precedence over the `embed_bitcode` flag
when lto is required by a target.

If both these flags would be supplied by the user, an error is
generated. However, this did not happen when lto was requested by the
target instead of the user.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@khanbilal732 khanbilal732 khanbilal732 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels

A-run-make Area: port run-make Makefiles to rmake.rs A-tidy Area: The tidy tool S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

amdgcn-amd-amdhsa target broken? Fails to build core

6 participants

@Flakebi @rustbot @rust-log-analyzer @jieyouxu @khanbilal732 @Mark-Simulacrum